feat!: Add PodSecurityContextBuilder::with_stackable_defaults#1205
feat!: Add PodSecurityContextBuilder::with_stackable_defaults#1205sbernauer wants to merge 1 commit into
PodSecurityContextBuilder::with_stackable_defaults#1205Conversation
| /// It is recommended to use the [`PodSecurityContextBuilder::with_stackable_defaults`] instead | ||
| /// (if possible). | ||
| pub fn stackable_default_pod_security_context() -> PodSecurityContext { | ||
| todo!("Lars needs to define the exact settings he wants"); |
There was a problem hiding this comment.
I would just leave the with_recommended_settings() function empty for now and keep the TODO in the issue, so that this pull request can be merged.
| pub fn new() -> Self { | ||
| Self::default() | ||
| /// Construct a new [`PodSecurityContextBuilder`] that is pre-filled with Stackable's defaults. | ||
| pub fn with_stackable_defaults() -> Self { |
There was a problem hiding this comment.
- I prefer a vendor neutral term like
with_recommended_settings, similar to https://search.nixos.org/options?channel=25.11&query=services.nginx.recommended. - I would keep the
newfunction and not force the settings yet:new().with_recommended_settings(). On the one hand, this pull request can then be merged now, and on the other hand, it is not guaranteed that we will be able to roll out this change on all operators at the same time. If we want to force the recommended settings later (which I would not do), we could create a function likenew_with_recommended_settingsand remove thenewfunction. - I would already use the builder functions here, to ensure that builder functions exist to override these settings.
- I asked for the
stackable_default_pod_security_contextfunction but it is not necessary and can be removed. The recommended settings can also be retrieved viaPodSecurityContextBuilder::new().with_recommended_settings().build().
There was a problem hiding this comment.
No objection against with_recommended_settings, so works for me. Hhowever to me this boils down to "is stackable-operator only for use or are other users also expected to use this?". I think we have different opinions in the team ^^ Discussion for later.
If we want to force the recommended settings later (which I would not do)
I though that was the point of this PR ^^ If some defaults don't work for a tool it can manually overwrite them.
I would keep the new function and not force the settings yet. On the one hand, this pull request can then be merged now
If we add a no-op function and rolling that out everywhere later one I have a bad feeling about e.g. setting the root filesystem to read only. If we define the defaults before merging we have a clearly breaking change. And not some operator-rs bump later on breaks everything silently. But no strong opinion.
| pod_security_context: PodSecurityContext, | ||
| } | ||
|
|
||
| impl PodSecurityContextBuilder { |
There was a problem hiding this comment.
There should also be defaults for the SecurityContextBuilder.
| /// It is recommended to use the [`PodSecurityContextBuilder::with_stackable_defaults`] instead | ||
| /// (if possible). | ||
| pub fn stackable_default_pod_security_context() -> PodSecurityContext { | ||
| todo!("Lars needs to define the exact settings he wants"); |
There was a problem hiding this comment.
I would just leave the with_recommended_settings() function empty for now and keep the TODO in the issue, so that this pull request can be merged.
2 participants
Description
Part of stackabletech/issues#645
Definition of Done Checklist
Author
Reviewer
Acceptance